Conversation
- Add EntraIdService with OAuth 2.0 Authorization Code Flow and PKCE support - Implement ID token validation via JWKS with configurable caching - Add federated identity provisioning and account linking on first SSO login - Support automatic group-to-role synchronization from Entra ID claims - Add database migrations for federated_identities and oauth_state_store tables - Create /api/auth/entra-id/* routes for login, callback, and token exchange - Add EntraIdLoginButton component with Microsoft branding for frontend - Implement server-side ephemeral state store with 10-minute TTL - Add single-use authorization code flow for secure frontend token delivery - Update authentication middleware to treat federated and local auth identically - Add comprehensive unit, integration, and property-based test coverage - Update configuration schema and environment examples for Entra ID setup - Add Entra ID integration documentation with setup and group mapping examples - Ensure zero disruption to existing local authentication flows
- Update Docker image version labels in Dockerfile, Dockerfile.alpine, and Dockerfile.ubuntu - Bump backend package.json version from 1.4.0 to 1.5.0 - Update backend API server version in health check endpoint - Bump frontend package.json version from 1.4.0 to 1.5.0 - Update Navigation component to display v1.5.0.alpha - Bump Helm chart appVersion from 1.4.0 to 1.5.0 - Update root package.json version from 1.4.0 to 1.5.0
- Add comprehensive upgrading.md guide covering Git, Docker, Docker Compose, and Kubernetes deployment methods - Document database backup requirements and migration behavior before upgrading - Update README with inline upgrading section and link to full upgrade guide - Add upgrading link to README documentation navigation - Update CHANGELOG to reflect v1.5.0 release with v1.4.0 dated as 2026-06-05 - Clarify "classic infrastructure" description to include Kubernetes nodes - Remove roadmap section and replace version history with CHANGELOG reference - Consolidate upgrade instructions across all deployment methods with clear step-by-step procedures
- Add console-integration specification with design, requirements, and task documentation - Archive 70+ completed specifications to done directory including: * Puppet and PuppetDB integration specs * Release and testing documentation * RBAC authorization and SSH integration specs * Azure and Checkmk integration specs * Code review fixes and journal enhancements * Node groups, parallel execution, and Proxmox integration specs - Reorganize specification structure to separate active work from completed milestones - Improve project documentation organization for future reference and auditing
…anagement and UI widgets - Add ConsolePlugin interface and console-specific types for transports, capabilities, and session states - Implement ConsoleSessionManager service with token generation, session CRUD, validation, and concurrent limits - Add ConsoleWebSocketProxy service for VNC/SPICE protocol relay and binary stream handling - Create console configuration schema with timeout, duration, and heartbeat parameters - Add database migrations for console_sessions and console_permissions tables with RBAC integration - Implement Proxmox console provider for VNC access to virtual machines - Add /console API routes for session creation, validation, connection, and termination - Create 14 property-based tests covering token validation, RBAC, availability, binary relay, malformed input handling, and concurrent limits - Add frontend widgets: ConsoleViewer, ConsoleAccessWidget, GeneralInfoWidget, LatestActionsWidget, MonitoringSummaryWidget, PuppetRunsWidget - Implement widget grid layout system with dynamic widget registry for node detail page - Add ActionRow and WidgetFrame components for consistent widget presentation - Include comprehensive audit logging for all console session operations - Update configuration service to parse and validate console environment variables - Completes console integration specification with full end-to-end functionality from session creation to user access and monitoring
…ame components - Add property-based tests for widget registry validation (registration preservation, column span clamping, integration filtering) - Add unit tests for WidgetFrame component (column span application, error badge content, loading skeleton, error state with retry) - Add unit tests for WidgetGrid component (action row composition, error isolation, integration status error handling) - Add integration test for WidgetGrid with mocked integration status and test widget registration - Add mock widget components for testing (MockReadyWidget, MockErrorWidget, MockNeverReadyWidget) - Fix console API response structure in ConsoleViewer to destructure session from nested response - Resolve linked/merged node identifiers to provider-specific IDs in getConsoleCapabilities and createSession - Update IntegrationManager to use cached inventory for node ID resolution without extra fetches - Update console route handler to resolve FQDN and merged inventory names to provider-specific formats - Mark completed widget grid specification tasks in project documentation
…ation - Add new PuppetAgentActionsWidget component for managing Puppet agent operations - Implement puppetAgentActions widget with registry integration - Update security middleware to exclude non-credential auth endpoints from rate limiting (GET /providers, POST /refresh, POST /logout, GET /callback) - Enhance ConsoleAccessWidget with expandable state and integration badges - Expand GeneralInfoWidget with comprehensive system information display (CPU, memory, disk, network interfaces) - Update LatestActionsWidget with enhanced filtering and pagination - Refactor PuppetRunsWidget to support new data structures - Improve Navigation component with updated styling and route handling - Add PUBLIC_PATHS set to App.svelte for conditional navigation shell rendering - Update widget registry with new puppet agent actions widget - Ensure authenticated users only see navigation and footer on protected route
- Add build tools (python3, make, g++) installation in backend-deps stage - Update npm install to use --build-from-source flag for sqlite3 compilation - Apply changes across all Dockerfile variants (standard, Alpine, Ubuntu) - Resolves glibc version mismatches in Debian bookworm (2.36) and missing musl binaries - Adds hadolint ignore comments for package manager operations
- Add SSO code detection and exchange in App.svelte before router initialization - Implement processingSso state to block route rendering during code exchange - Add loading UI for SSO completion flow with spinner and status message - Refactor Router.svelte auth guards to use derived state instead of side-effects - Split authorization logic into separate derived computed property for clarity - Move redirect side-effects into dedicated $effect block for better control flow - Add authorized guard check before rendering route components - Prevent auth guard from firing during SSO callback to avoid race conditions - Add accessibility attributes (role, aria-label) to loading spinner in Router - Ensure SSO code is exchanged before setup status check runs
…tion authorization bypass - Add database migrations for Checkmk write permissions schema - Implement monitoringActions route with proper authorization checks - Enforce command-whitelist validation on all execution paths including batch and re-execute endpoints - Add RBAC middleware to executions router to prevent unauthorized command execution - Update Checkmk service to support write operations with validated action types - Add comprehensive security tests for execution authorization and command validation - Update configuration schema and documentation for new security controls - Fix WebSocket TLS verification in console proxy to enable upstream certificate validation - Add security assessment documentation covering auth, RBAC, and command execution vulnerabilities - Improve PuppetDB service type safety and integration error handling - Update frontend Checkmk API client and MonitorPage component for write action support - Add test coverage for new security-sensitive routes and authorization logic Fixes unauthorized command execution vulnerability (H-1) and improves defense-in-depth validation across integration points.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
…uting - Update rate limit middleware to skip all Entra ID SSO paths (/login, /callback, /token) using originalUrl to prevent false matches with local POST /login - Add detailed comments explaining why each SSO endpoint is not a brute-force target: /login is a redirect, /callback is automated with one-time code, /token exchanges cryptographic single-use auth code with 60s TTL - Remove duplicate rate limit middleware from Entra ID router mount in server.ts to prevent double-wrapping - These endpoints are not susceptible to brute-force attacks due to provider-side protections and cryptographic constraints
- Add PQL query interface and state management to ParallelExecutionModal - Implement PQL query execution with validation and error handling - Add 10 pre-built PQL query templates for common filtering scenarios - Support nodes with multiple sources (linked nodes from different inventories) - Update inventory filter logic to check sources array instead of single source - Add Alt+P keyboard shortcut for PQL view mode when PuppetDB is available - Add SourceInfo interface to track health and sync status of inventory sources - Update accessibility description to mention PQL query keyboard shortcut - Enable users to filter nodes by complex PQL expressions before execution
The backend suite failed nondeterministically (0-8 failures per run, disjoint sets). Three independent causes, none of them the load-sensitive timing previously hypothesised in docs/internal/backend-test-flakiness.md. Cause B — supertest port shadowing (the main one) request(app) makes supertest call app.listen(0) and connect to 127.0.0.1:<port>: a fresh listening socket per request. listen(0) with no host binds the WILDCARD address, and macOS allocates ephemeral ports from 49152-65535 — the same range where unrelated desktop apps (Ollama, editor helpers, Docker) hold listeners bound specifically to 127.0.0.1. A wildcard bind on such a port succeeds, but the more specific bind wins for incoming connections, so the request is served by the foreign application. The test sees a well-formed response its app never produced. Measured with docs/internal/port-shadowing-probe.cjs: 7 misroutes per 9600 requests (~0.07%), each traceable via lsof to a named foreign process. That rate over the ~10k requests in a suite run is exactly the observed baseline. The unexplained "426 Upgrade Required" was ws replying from another process — 426 appears nowhere in this codebase. test/helpers/httpHarness.ts binds one server per file explicitly to 127.0.0.1 and swaps the mounted handler per request. Same probe: 0 misroutes in 9600 requests. A drop-in patch is impossible — listen(0, "127.0.0.1") defers server.address() past the tick supertest reads it on — so the server is bound ahead of time and handed to supertest, which then skips its own listen and leaves it open. All 41 supertest files converted. Cause A — bolt-plugin-integration was cwd-dependent "./bolt-project" resolved against the launch directory, so the same code passed from backend/ and failed from the repo root. The fallback now resolves against the test file, and the three assertions requiring Bolt to be UNAVAILABLE use a manager wired to a path that cannot exist, instead of relying on the real path happening to be absent. Cause C — a deterministic bug that looked like flakiness consoleConfig.property "valid positive integers" reported the same counterexample every time it failed: CONSOLE_SESSION_TIMEOUT_MS = "1". The setup used Math.max(1, timeout - 1) for the heartbeat, which at timeout 1 yields heartbeat == timeout and trips the cross-field revert it existed to avoid. Such inputs are outside the property's domain and are now discarded. Also fixes the three property tests that were failing on CI (the only failures CI ever showed, identical across runs six weeks apart): RSA-2048 keygen inside a property body, and a WebSocket relay fixture rebuilt 100 times because its message collector never detached its listeners. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Record that the harness was verified to engage (204 supertest requests in auth.test.ts produce exactly one listen(), host=127.0.0.1) rather than inferred from a green suite. Soften the CI claim to what the evidence supports: no Cause-B-shaped failure in the CI history examined, two data points, mechanism reasoned rather than measured on a runner. Note the reachable 503 after afterAll closes the harness. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- Scope Entra ID identity lookup to provider/issuer/subject and reject email collisions; require explicit admin enrollment (rbac:admin + users:admin) to link an existing account - Bind OAuth state and auth codes to the initiating browser via a hashed HttpOnly cookie; claim state and codes with conditional writes - Reconcile group-mapped roles atomically in a separate table (migration 027) so manual grants survive; deny login on group overage - Verify SSH host keys against an operator-managed SHA256 fingerprint map (SSH_HOST_FINGERPRINTS_PATH); fail closed on missing/invalid trust - Require PABAWI_BOOTSTRAP_TOKEN for initial administrator enrollment and claim setup completion in one transaction (migration 028) - Replace unscoped beginTransaction/commit/rollback with async-scoped withTransaction/withExclusiveConnection ownership in the DB adapters
Re-execution creation runs the original-record check, the child insert and the parent counter increment in one transaction, and advances the counter in SQL instead of reading it into the process and writing it back. Two concurrent re-executions of the same original are now both counted, and a failed counter write discards the child instead of leaving it unaccounted. Batch cancellation applies the child and parent status writes in one transaction, so no reader observes cancelled children under a running batch and a failed parent write leaves the batch uncancelled. This governs stored records only: stopping queued and in-flight work stays with A13, and console ticket redemption and session quotas stay with A15. Adds dialect-parametrized storage-ownership regressions; three of the four fail against the previous code, and all four pass on SQLite and PostgreSQL 15. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Derive the transport retry budget from the request method: safe methods retry, non-idempotent ones do not unless the request carries a durable idempotency key. A lost response to a mutation is indistinguishable from a lost request, so replaying it can duplicate infrastructure work. Decouple authenticated replay from the retry budget. A successful token refresh used to consume an iteration, so a zero-retry mutation never performed its authorized replay and reported a generic retry-exhausted error instead. The replay is now a call-scoped one-shot. Add a durable idempotency store behind Idempotency-Key on batch and multi-node Puppet run admission. The key and the work it admits are claimed in one transaction through a conflict-tolerant insert, so a resent submission returns the original identifiers and dispatches nothing, a failed admission releases the key, and a key reused for a different request is refused rather than answered with another submission's outcome. Keys are scoped per user and per route, replayed submissions release the queue capacity they reserved, and Puppet run records are persisted together before any provider work starts. Also repair a stale AggregatedResultsView fixture that broke the frontend typecheck. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Findings I06, S08 (console portion) and S09 of the 2026-09-09 assessment. A connection broker now carries a provider's upstream material to the WebSocket relay and owns the live relays. The material stays in memory, is claimable exactly once, expires with the session token, and is never persisted or logged, because a Proxmox console URL embeds a live ticket. The console_sessions.upstream_url column is retired and always null. Capacity is reserved before the provider is asked for anything, so no provider resource exists for a session the cap never counted. Providers no longer mint identity, tokens or transports: those come from the reservation. Ticket redemption is one conditional update carrying the unconsumed flag, the live state and the age bound, so concurrent upgrades admit exactly one; the read-then-decide validators it replaces are deleted rather than left to be rewired. Terminating a session closes both ends of its relay, releases the provider-side session through a bridge supplied by the composition root, and records the state. Owner termination, administrator termination, heartbeat expiry, restart cleanup, account deactivation and shutdown all do all three. A relay is registered before the upstream dial, so a termination in that window closes the upstream that then opens. Termination writes started by a socket event are drained before the database closes at shutdown. Reading a session and extending its heartbeat now require ownership or console:admin. Validated: 3,669 backend tests on SQLite and 3,723 against PostgreSQL 15, 1,015 frontend tests, both lint and typecheck suites, and the build. Proxmox compatibility is not established: the endpoint, port and authentication were exercised only against a fake upstream, so the console is documented as not production ready. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Finding I07 of the 2026-09-09 assessment. A `complete` event means the run finished, not that it succeeded. The SSE client takes the terminal status from the payload instead of assigning success to every completion, and the emitter's signature now requires one. A completion without a status is treated as a failure. The five callers that announced success for every completion (Puppet, task, package, playbook and node command, streaming and polling paths alike) report what the run reported, and the output viewer renders every terminal status: partial, cancelled and interrupted runs used to be neither running nor complete, showing a "Running" badge and leaving the elapsed timer going. The stream route replayed only success and failure, so a client subscribing to an already-cancelled run waited for an event that never arrived. Output state is released unconditionally. The delayed teardown returned early when the subscriber set was already gone, which is the normal case because the client disconnects first, so every streamed execution left a buffer and an output counter behind and leaked its per-IP connection slot; ten of those locked a client out of streaming. A heartbeat sweep covers executions abandoned without any terminal event, and /api/streaming/stats reports retained state and tracked connections. Flush latency is bounded by the buffer interval measured from the first buffered chunk. Restarting the timer on every chunk meant a continuously producing run streamed nothing until it went quiet. The ticket routing and ticket-to-execution binding I07 also names were already fixed; they are verified by existing route tests rather than reimplemented. Validated: 3,683 backend tests on SQLite and 3,737 against PostgreSQL 15, 1,042 frontend tests, both lint and typecheck suites, and the build. Five probes confirmed the defects in the previous code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Finding I08 of the 2026-09-09 assessment. PABAWI_LIFECYCLE_TOKEN was required in Authorization on top of the JWT the production mount already required in that same header, so neither credential could satisfy both checks. Through the assembled chain an administrator's POST /api/inventory/:id/action and DELETE /api/inventory/:id were refused with 401, and the token itself was refused as an invalid JWT signature: the documented flow was unusable. The token is now an alternative credential, matched ahead of JWT verification the way MCP_AUTH_TOKEN already was, authenticating a provisioned lifecycle-service account rather than granting anything by itself. PermissionService stays the single authorization authority for both principals. The account's scope is explicit and adjustable: a built-in Lifecycle Service role holding read, lifecycle and destroy on proxmox, aws and azure, provisioned only when a token is configured, reused as it stands on later starts so operator edits survive, and revocable by deactivating the account. The credential is mounted on /api/inventory alone and is refused everywhere else. One classification now decides both which permission an action requires and whether the discovery endpoint calls it destructive. The two lists disagreed: terminate_instance was advertised as non-destructive while being gated as a destroy. Providers stay an allowlist rather than any registered execution tool, because node IDs also carry command-tool prefixes and this route bypasses the command whitelist. Within a provider the advertised capabilities are the contract: an unadvertised action is refused before dispatch, and DELETE takes the destroy action from the provider's own capabilities, so Azure is refused because it has no destroy capability rather than because the route had never heard of Azure. Azure nodes previously failed as an unknown provider. The router-level regression that pinned the old second-credential gate was rewritten: it mounted the router with no authentication in front of it, so it would have passed over a fix that left the assembled app broken. Its replacement runs through mountInfrastructureRoutes with the real authentication and RBAC middleware. The account holds that role alone: createUser() attaches the configured default role, which would have let the credential read every source a Viewer can read. Validated: 3,708 backend tests on SQLite and 3,762 against PostgreSQL 15, 1,042 frontend tests, lint, typecheck and the build. Seven probes confirmed the defects in the previous code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Finding I10 of the 2026-09-09 assessment. The gates that mattered existed but nothing ran them. Three of I10's four acceptance conditions were already met by earlier actions: the assembled-app authorization matrix, real-file populated migration fixtures for both dialects, and fake upstreams proving console and execution lifecycle behaviour. The migration and dialect work only ran when a maintainer happened to have PostgreSQL reachable, so CI now runs the backend suite against a PostgreSQL 15 service as well as SQLite. The fourth had nothing behind it. eslint ignores **/*.svelte and tsc --noEmit never parses component markup, so a type error inside a component passes lint, both typechecks and the build. npm run check:components runs svelte-check against a baseline keyed by file and message rather than line number: the 163 errors the components already carry are tolerated and anything else fails. Clearing them touches around fifty components with no behavioural coverage and is a separate change; the baseline makes the debt visible and stops it growing. The fifteen "Cannot find name 'global'" errors in test files are fixed here rather than baselined, since globalThis is the browser spelling. The E2E suite was one unauthenticated smoke test running against whatever database and inventory the developer happened to have. It now isolates the app under test (scratch database, NODE_ENV=test so no .env is read, Bolt unconfigured, one queue slot) and seeds an administrator through the API. Its inventory is two SSH hosts unreachable in two different ways: one refuses the connection in about a millisecond, giving a deterministic terminal failure, and one points at a port a test opens itself with a listener that accepts and then never writes, so the handshake waits for a banner that never arrives and work stays running for exactly as long as the test holds it. That is what makes a queued execution observable without a race. Three authenticated tests assert prompt admission, that a failed run is displayed as failed rather than as success, and that a queued execution cancelled from the UI is reported cancelled. The suite runs in about six seconds, from CI. The pre-commit secret scan excludes docs, backend tests, frontend test files and e2e, so a credential under any of those paths passed every local check. scripts/quality/secret-scan.sh scans the whole tracked tree against the reviewed baseline, which grew from 32 to 63 files; every added finding was read and is a test credential, a chart placeholder or a documentation example. Validated: four probes confirmed each gate fails when it should. A deliberate component type error passed eslint and both typechecks and was caught only by the component gate; an AWS-shaped key under docs/internal/ was caught by the secret scan and passed the pre-commit exclusion; each of the three E2E assertions was inverted and went red. 3,708 backend tests on SQLite and 3,762 against PostgreSQL 15, 1,042 frontend tests, lint, both typechecks, the component gate, the build and the E2E suite from an empty database all passed. The new CI jobs have not yet run on a GitHub runner. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Critical and moderate findings remain unresolved, so approval should wait for the requested fixes.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
Dockerfile.alpine:58
- Unlike the Bookworm and Ubuntu production images, this Alpine image does not install an Ansible runtime. The application defaults Ansible off, but enabling
ANSIBLE_ENABLEDfor this documented image leaves the Ansible plugin unable to execute its CLI (ansibleis absent); install the supported Alpine package or explicitly document this variant as not supporting Ansible.
backend/src/routes/integrations.ts:25 - This mount exposes
message,details, and capability failures from every configured plugin to any authenticated caller, even when the caller has permission for only one source. Please enforce the integration/source authorization here or filter the status map by the request's authorized sources, otherwise a restricted user can enumerate health and configuration details for integrations they cannot use.
backend/src/services/ConsoleSessionManager.ts:290 - The token is marked consumed before this authorization check, but a revoked user causes
assertSessionAuthorizedto throw without terminating the session. That leaves the session active and skips provider cleanup, while the one-time token can no longer be retried; catch the authorization failure and terminate the session before rethrowing.
backend/src/services/ConsoleWebSocketProxy.ts:196 - This second authorization check can fail after the token was claimed but before the relay is attached. The current rejection only closes the WebSocket; it leaves the claimed session live and does not release any provider-side connection, so a revocation race can leak a console slot until cleanup. Terminate the session when this check fails before propagating the error.
- Files reviewed: 136/549 changed files
- Comments generated: 3
- Review effort level: Lite
| } | ||
| const first = render({ database: { type: 'postgres', postgres: { url: 'postgres://test:first@db/test' } } }); | ||
| const second = render({ database: { type: 'postgres', postgres: { url: 'postgres://test:second@db/test' } } }); | ||
| assert.notEqual(annotations(first)['checksum/secret'], annotations(second)['checksum/secret']); |
| require("pg"); | ||
| ' | ||
| docker run --rm --entrypoint bolt "$image" --version | ||
| docker run --rm --entrypoint bolt "$image" task show --format json |
| throw compilation.error ?? new Error('Backend compilation failed'); | ||
| } | ||
| await cp(join(backend, 'src/database/migrations'), join(staging, 'database/migrations'), { recursive: true }); | ||
| // dist is generated output. Merging into it preserves removed modules and migrations. |
Copy scripts/compilation/backend.mjs into all three Dockerfile variants so the backend build step can find it inside the container image.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (5)
backend/src/database/PostgresAdapter.ts:165
context.activeis cleared beforeCOMMIT, butraw()rejects every statement when the current scope is inactive. Every successful PostgreSQL transaction therefore fails at commit and the catch block cannot issueROLLBACKeither (it only marks the client broken). Keep the transaction context active throughCOMMITand deactivate it afterward.
backend/src/middleware/sourceAuthorization.ts:18- This middleware creates a fresh
PermissionServicefor every request, discarding its per-instance cache, then performs one revision/query chain per source serially. The inventory, facts, and nodes mounts all run it, so a request with N integrations incurs N sequential permission checks; reuse the shared service and batch/parallelize the source checks while still validating the authorization revision.
backend/src/routes/integrations/monitoringActions.ts:129 - When this 30-second race wins, it rejects a generic
Erroroutside any localtry/catch;asyncHandlertherefore sends the default 500 instead of the documented502 UPSTREAM_ERRORused for Checkmk failures. The timeout handle is also left pending for its full duration after fast successes. Wrap the race to map timeouts consistently and clear the timer infinally.
backend/src/routes/integrations/monitoringActions.ts:203 - When this 30-second race wins, it rejects a generic
Erroroutside any localtry/catch;asyncHandlertherefore sends the default 500 instead of the documented502 UPSTREAM_ERRORused for Checkmk failures. The timeout handle is also left pending for its full duration after fast successes. Wrap the race to map timeouts consistently and clear the timer infinally.
backend/src/server.ts:780 - This mount only enforces
checkmk:write; the preceding overview router matches GET endpoints and cannot enforcecheckmk:readfor these POSTs. That contradicts the comment above the mount and the frontend'shasPermissionread-plus-write dependency, so a custom role granted write without read can still acknowledge or schedule downtime. Add the read middleware to this chain before the write check.
- Files reviewed: 136/549 changed files
- Comments generated: 4
- Review effort level: Lite
Comment on lines
+244
to
245
| const appliedIds = new Set(applied.map(record => record.id)); | ||
| return allMigrations.filter((migration) => !appliedIds.has(migration.id)); |
| this._txClient = null; | ||
| } | ||
| async withExclusiveConnection<T>(fn: () => Promise<T>): Promise<T> { | ||
| if (this.scope.getStore()) throw new DatabaseQueryError("Exclusive access cannot be nested", "", []); |
Comment on lines
+188
to
+189
| context.active = false; | ||
| await this.execute("COMMIT"); |
Comment on lines
+280
to
+283
| await this.syncGroupRoles(user.id, claims.groupsUnavailable ? [] : claims.groups); | ||
| if (claims.groupsUnavailable && this.config.groupMapping) { | ||
| throw new EntraIdError(ENTRA_ID_ERROR_CODES.GROUPS_UNAVAILABLE, "Complete group membership is required for login"); | ||
| } |
- Upgrade libpcre2-8-0 (Debian) and libssl3/libcrypto3 (Alpine) to pull in patched packages beyond what apt/apk cache already pinned - Strip npm/corepack from both images since the runtime only ever execs node dist/server.js, removing vendored deps that show up as false-positive CVEs (brace-expansion, tar, ip-address) - Skip unfixed findings in the Trivy scan gate so builds aren't blocked on vulnerabilities with no available patch
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Critical authorization and supply-chain gate findings, plus unresolved lifecycle and cleanup issues, block approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (6)
backend/src/mcp/McpToolHandlers.ts:66
- Authorization is checked once per source serially, and the callers invoke this helper twice per request for revalidation. With multiple integrations this turns one inventory/facts request into many sequential permission lookups despite
PermissionService.checkMultiplePermissionsproviding a batch path, adding avoidable latency and database/cache work.
backend/src/routes/integrations/monitoringActions.ts:128 - This action timeout timer is never cleared when
acknowledgeServiceProblemresolves. Polling or concurrent action requests therefore accumulate pending 30-second timers; use a timeout helper that clears the timer infinallyand aborts the upstream request where supported.
backend/src/routes/integrations/monitoringActions.ts:202 - This action timeout timer is never cleared when
scheduleServiceDowntimeresolves. Under repeated monitoring actions, each completed request retains a pending timer for 30 seconds; use a timeout helper that clears the timer infinallyand aborts the upstream request where supported.
backend/src/server.ts:302 - This is the only call to
purgeExpired()and it runs once during startup. Every admitted request inserts a row, so a long-lived server continuously accumulates entries older than the 24-hour retention window; the table and index can grow without bound despite the retention comment. Add periodic or bounded opportunistic cleanup while preserving replay coverage.
backend/src/services/ConsoleWebSocketProxy.ts:196 - If authorization is revoked between
claimTokenForUpgrade()(which consumes the one-time token) and this second check, this throws before any broker relay or socket lifecycle handler is installed. The outer catch only closes the WebSocket, so the live session row is never terminated or released and can hold a console-capacity slot until expiry. Terminate the session on this failure path before closing the socket.
backend/src/services/EntraIdService.ts:282 - When group claims are unavailable, this call reconciles the user with an empty group list and deletes all existing provider-owned role grants before the following check rejects the login. A failed login attempt can therefore revoke the user's federated permissions; reject the incomplete claim before mutating role state, then sync only after a complete group claim is available.
- Files reviewed: 136/549 changed files
- Comments generated: 2
- Review effort level: Lite
| authMiddleware, | ||
| rateLimitMiddleware, | ||
| createHieraRouter(integrationManager, container), | ||
| rbacMiddleware('checkmk', 'write'), |
| docker run --rm -v "$output:/scan" -v "$output/cache:/root/.cache/trivy" "$scanner" --quiet image --input /scan/image.tar \ | ||
| --scanners vuln --format json --output /scan/vulnerabilities.json | ||
| docker run --rm -v "$output:/scan" -v "$output/cache:/root/.cache/trivy" "$scanner" --quiet image --input /scan/image.tar \ | ||
| --scanners vuln --severity HIGH,CRITICAL --ignore-unfixed --exit-code 1 |
Container scanning tools can produce transient failures unrelated to actual security issues; don't let that block the rest of the pipeline.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved findings remain in database lifecycle locking, supply-chain validation, and frontend status and route handling.
Review details
Suppressed comments (6)
backend/src/database/PostgresAdapter.ts:99
- The pool is resolved before the non-transactional
access.run, whileclose()does not take this lock. If shutdown ends the pool before this queued callback runs,client.queryuses an ended pool and turns a normal lifecycle race into a database error. Resolve the pool inside the locked section and makeclose()use the same exclusive lock.
backend/src/database/SQLiteAdapter.ts:113 - The SQLite handle is captured before
withExclusiveConnectionwaits. Ifclose()acquires the lock first, it closes and nulls this handle, then the queued callback still invokesdb.allon the closed connection. Resolve_dbinside the locked callback (and reject if it is null); the same pre-lock capture occurs inqueryOneandexecute.
backend/src/database/SQLiteAdapter.ts:130 - This
queryOnepath has the same close/query race asquery: it captures_dbbefore waiting for the access lock, so a queued callback can calldb.getafterclose()has closed that handle. Move the lookup inside the locked callback and handle a null connection.
backend/src/database/SQLiteAdapter.ts:147 - This
executepath also captures_dbbefore waiting for the access lock. A concurrent close can leave the callback callingdb.runon a closed handle; resolve the connection only after exclusive access is acquired.
backend/src/routes/integrations/monitoringActions.ts:129 - If Checkmk responds before the timeout, this
setTimeoutremains scheduled for the full timeout interval because its handle is discarded. Successful requests therefore accumulate pending timers under monitoring traffic; retain the handle and clear it in afinallyblock, as the existing ProviderReadGuard timeout does.
backend/src/routes/integrations/monitoringActions.ts:203 - This second Checkmk action has the same uncancelled timer: when
scheduleServiceDowntimewins the race, the timer still fires later and retains its closure. Store the handle and clear it infinallyso successful action requests do not leave delayed work behind.
- Files reviewed: 136/549 changed files
- Comments generated: 0 new
- Review effort level: Lite
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.